Upgrade jsonrpc to 0.18.0 #9547

bkchr · 2021-08-12T10:09:44Z

I think this says all :P

polkadot companion: paritytech/polkadot#3633

I think this says all :P

athei · 2021-08-12T10:14:31Z

Should we revert this then: #9518 ?

andresilva

LGTM, only question is about changes from bounded to unbounded channels in a couple of places.

andresilva · 2021-08-12T10:29:33Z

utils/browser/src/lib.rs

 	#[wasm_bindgen(js_name = "rpcSend")]
 	pub fn rpc_send(&mut self, rpc: &str) -> js_sys::Promise {
-		let rpc_session = RpcSession::new(mpsc01::channel(1).0);
+		let rpc_session = RpcSession::new(mpsc::unbounded().0);


Any particular reason why this and below was changed to unbounded?

andresilva · 2021-08-12T10:33:34Z

client/rpc-api/src/metadata.rs

 impl Metadata {
 	/// Create new `Metadata` with session (Pub/Sub) support.
-	pub fn new(transport: mpsc::Sender<String>) -> Self {
+	pub fn new(transport: mpsc::UnboundedSender<String>) -> Self {


Why change to unbounded?

This comes from jsonrpc 🤷 I only having adapted to the requirements :D

It was unbounded before too, it's just the name that is more explicit now.

I think this change causes the related BEEFY Update jsonrpc PR to fail.

We run into the old chicken-and-egg problem. For the BEEFY PR to compile it needs an updated version of sc_rpc::Metadata

The PR is failling because it is using master of Substrate that does not yet have this pr.

Just approve the pr

The PR is failling because it is using master of Substrate that does not yet have this pr.

Yep, that is what I wrote ...

andresilva · 2021-08-12T10:41:17Z

client/rpc/src/system/tests.rs

-	let mut runtime = tokio::runtime::current_thread::Runtime::new().unwrap();
-	runtime.block_on(rx).unwrap()
-}
+#[cfg(test)]


This whole file is already only compiled when config is test.

Fuck xD I'm an idiot xD

andresilva · 2021-08-12T10:42:48Z

client/rpc/src/state/state_light.rs

-						false => None,
-					}
+
+					version_differs.then(|| new_version.clone())


andresilva · 2021-08-12T10:49:20Z

client/rpc/src/author/mod.rs

+				warn!("Failed to submit extrinsic: {}", err);
+				// reject the subscriber (ignore errors - we don't care if subscriber is no longer there).
+				let _ = subscriber.reject(err.into());
+				return


This is a bit shittier as we're duplicating the same error handling as below.

bkchr · 2021-08-12T13:48:06Z

Should we revert this then: #9518 ?

Good question

athei · 2021-08-12T14:04:19Z

Should we revert this then: #9518 ?

Good question

I think at least we should change the version there (in this PR) to >= 19 so that we get minor version bumps via the bot.

tomusdrw · 2021-08-12T18:25:17Z

client/rpc-api/src/metadata.rs

 impl Metadata {
 	/// Create new `Metadata` with session (Pub/Sub) support.
-	pub fn new(transport: mpsc::Sender<String>) -> Self {
+	pub fn new(transport: mpsc::UnboundedSender<String>) -> Self {


It was unbounded before too, it's just the name that is more explicit now.

This reverts commit f2b3997.

bkchr · 2021-08-12T21:11:48Z

Should we revert this then: #9518 ?

Good question

I think at least we should change the version there (in this PR) to >= 19 so that we get minor version bumps via the bot.

I have reverted it

bkchr · 2021-08-13T06:46:02Z

bot merge

ghost · 2021-08-13T06:46:06Z

Trying merge.

* Upgrade jsonrpc to 0.18.0 I think this says all :P * 🤦 * Fmt etc * Fix tests * Fix tests again... * Better impl * Revert "Tell dependabot to ignore jsonrpc-* updates (#9518)" This reverts commit f2b3997.

Upgrade jsonrpc to 0.18.0

6533cb1

I think this says all :P

bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 12, 2021

bkchr requested review from andresilva, athei and sorpaas as code owners August 12, 2021 10:09

andresilva approved these changes Aug 12, 2021

View reviewed changes

bkchr added 3 commits August 12, 2021 15:09

Merge remote-tracking branch 'origin/master' into bkchr-upgrade-jsonrpc

1827cbf

🤦

fbf7cc0

Fmt etc

cc09f74

bkchr requested a review from kianenigma as a code owner August 12, 2021 13:46

bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Aug 12, 2021

Fix tests

3eed63d

tomusdrw approved these changes Aug 12, 2021

View reviewed changes

bkchr added 3 commits August 12, 2021 23:06

Fix tests again...

6574769

Better impl

a00166d

Revert "Tell dependabot to ignore jsonrpc-* updates (#9518)"

20b0f76

This reverts commit f2b3997.

bkchr requested a review from a team as a code owner August 12, 2021 21:11

ghost merged commit 7342a2f into master Aug 13, 2021

ghost deleted the bkchr-upgrade-jsonrpc branch August 13, 2021 06:46

JoshOrndorff added a commit to moonbeam-foundation/frontier that referenced this pull request Aug 24, 2021

revert changes corresponding to jsonrpc 18 and paritytech/substrate#9547

2e30a0a

chevdor mentioned this pull request Sep 7, 2021

chore: update beefy to substrate branch polkadot-v0.9.10 paritytech/grandpa-bridge-gadget#285

Merged

JoshOrndorff added a commit to moonbeam-foundation/frontier that referenced this pull request Sep 16, 2021

revert changes corresponding to jsonrpc 18 and paritytech/substrate#9547

c809029

JoshOrndorff added a commit to moonbeam-foundation/frontier that referenced this pull request Sep 28, 2021

revert changes corresponding to jsonrpc 18 and paritytech/substrate#9547

98d253f

JoshOrndorff added a commit to moonbeam-foundation/frontier that referenced this pull request Sep 28, 2021

revert changes corresponding to jsonrpc 18 and paritytech/substrate#9547

d21e61d

boundless-forest mentioned this pull request Dec 7, 2021

DVM Update Trace 2 darwinia-network/darwinia-common#981

Closed

This pull request was closed.

Upgrade jsonrpc to 0.18.0 #9547

Upgrade jsonrpc to 0.18.0 #9547

Uh oh!

Conversation

bkchr commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

athei commented Aug 12, 2021

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Aug 12, 2021

Uh oh!

athei commented Aug 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Aug 12, 2021

Uh oh!

bkchr commented Aug 13, 2021

Uh oh!

ghost commented Aug 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bkchr commented Aug 12, 2021 •

edited

Loading